@llvm/pr-subscribers-lldb

Author: John Harrison (ashgti)

diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 51f9da854f4b6..827ebb1465938 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -26,6 +26,7 @@
 #include "lldb/API/SBListener.h"
 #include "lldb/API/SBProcess.h"
 #include "lldb/API/SBStream.h"
+#include "lldb/API/SBTarget.h"
 #include "lldb/Utility/IOObject.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/lldb-defines.h"
@@ -719,23 +720,7 @@ lldb::SBTarget DAP::CreateTarget(lldb::SBError &error) {
   return target;
 }
 
-void DAP::SetTarget(const lldb::SBTarget target) {
-  this->target = target;
-
-  if (target.IsValid()) {
-    // Configure breakpoint event listeners for the target.
-    lldb::SBListener listener = this->debugger.GetListener();
-    listener.StartListeningForEvents(
-        this->target.GetBroadcaster(),
-        lldb::SBTarget::eBroadcastBitBreakpointChanged |
-            lldb::SBTarget::eBroadcastBitModulesLoaded |
-            lldb::SBTarget::eBroadcastBitModulesUnloaded |
-            lldb::SBTarget::eBroadcastBitSymbolsLoaded |
-            lldb::SBTarget::eBroadcastBitSymbolsChanged);
-    listener.StartListeningForEvents(this->broadcaster,
-                                     eBroadcastBitStopEventThread);
-  }
-}
+void DAP::SetTarget(const lldb::SBTarget target) { this->target = target; }
 
 bool DAP::HandleObject(const Message &M) {
   TelemetryDispatcher dispatcher(&debugger);
@@ -1489,17 +1474,31 @@ void DAP::ProgressEventThread() {
 }
 
 // All events from the debugger, target, process, thread and frames are
-// received in this function that runs in its own thread. We are using a
-// "FILE *" to output packets back to VS Code and they have mutexes in them
-// them prevent multiple threads from writing simultaneously so no locking
-// is required.
+// received in this function that runs in its own thread.
 void DAP::EventThread() {
   llvm::set_thread_name(transport.GetClientName() + ".event_handler");
-  lldb::SBEvent event;
+
+  // Configure the debugger listener for all events lldb-dap is interested in.
   lldb::SBListener listener = debugger.GetListener();
-  broadcaster.AddListener(listener, eBroadcastBitStopEventThread);
-  debugger.GetBroadcaster().AddListener(
-      listener, lldb::eBroadcastBitError | lldb::eBroadcastBitWarning);
+  listener.StartListeningForEventClass(
+      debugger, lldb::SBTarget::GetBroadcasterClassName(),
+      lldb::SBTarget::eBroadcastBitBreakpointChanged |
+          lldb::SBTarget::eBroadcastBitModulesLoaded |
+          lldb::SBTarget::eBroadcastBitModulesUnloaded |
+          lldb::SBTarget::eBroadcastBitSymbolsLoaded |
+          lldb::SBTarget::eBroadcastBitSymbolsChanged);
+  listener.StartListeningForEventClass(
+      debugger, lldb::SBProcess::GetBroadcasterClassName(),
+      lldb::SBProcess::eBroadcastBitStateChanged |
+          lldb::SBProcess::eBroadcastBitSTDOUT |
+          lldb::SBProcess::eBroadcastBitSTDERR);
+  listener.StartListeningForEvents(debugger.GetBroadcaster(),
+                                   lldb::SBDebugger::eBroadcastBitError |
+                                       lldb::SBDebugger::eBroadcastBitWarning);
+  // Listen for the lldb-dap stop event.
+  listener.StartListeningForEvents(broadcaster, eBroadcastBitStopEventThread);
+
+  lldb::SBEvent event;
   bool done = false;
   while (!done) {
     if (listener.WaitForEvent(1, event)) {
@@ -1633,8 +1632,8 @@ void DAP::EventThread() {
             SendJSON(llvm::json::Value(std::move(bp_event)));
           }
         }
-      } else if (event_mask & lldb::eBroadcastBitError ||
-                 event_mask & lldb::eBroadcastBitWarning) {
+      } else if (event_mask & lldb::SBDebugger::eBroadcastBitError ||
+                 event_mask & lldb::SBDebugger::eBroadcastBitWarning) {
         lldb::SBStructuredData data =
             lldb::SBDebugger::GetDiagnosticFromEvent(event);
         if (!data.IsValid())
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index c2e4c2dea582e..c76f6d2ba1ffc 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -334,8 +334,7 @@ struct DAP {
   ///     An SBTarget object.
   lldb::SBTarget CreateTarget(lldb::SBError &error);
 
-  /// Set given target object as a current target for lldb-dap and start
-  /// listeing for its breakpoint events.
+  /// Set given target object as a current target for lldb-dap.
   void SetTarget(const lldb::SBTarget target);
 
   bool HandleObject(const protocol::Message &M);
diff --git a/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
index 0293ffbd0c922..167ddb9eb73ed 100644
--- a/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
@@ -96,7 +96,9 @@ Error AttachRequestHandler::Run(const AttachRequestArguments &args) const {
       if (llvm::Error err = dap.RunAttachCommands(args.attachCommands))
         return err;
 
-      dap.target = dap.debugger.GetSelectedTarget();
+      // The custom commands might have created a new target so we should use
+      // the selected target after these commands are run.
+      dap.SetTarget(dap.debugger.GetSelectedTarget());
 
       // Validate the attachCommand results.
       if (!dap.target.GetProcess().IsValid())
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
index 93bc80a38e29d..7de582ee94320 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
@@ -209,7 +209,7 @@ llvm::Error BaseRequestHandler::LaunchProcess(
 
       // The custom commands might have created a new target so we should use
       // the selected target after these commands are run.
-      dap.target = dap.debugger.GetSelectedTarget();
+      dap.SetTarget(dap.debugger.GetSelectedTarget());
     }
   }
 

Summarizing the problem to make sure I understand: with the new launch/attach flow, we set the breakpoints in the dummy target before the actual target is created. If the target is created through launch commands, we're calling SetTarget with the currently selected target after we're done running the launch commands. By that time, we may have already missed events belong to the new target.

With this PR, we mimic what the default event handler does, which is listen to all target events. I think the problem with this approach is that now we're going to be broadcasting events for targets that are not the focus of the current debug session (i.e. the selected target). It should be easy to verify this by running a script that creates a second target and seeing if we now broadcast the events. If my theory is correct, then we would have to filter events not belonging to the selected target.

Interestingly, I was thinking about multi-target/multi-process support in DAP earlier this week. I had someone reach out to ask questions about this. I'll file an issue to track this, but we should keep that use case in mind as we design this.

Summarizing the problem to make sure I understand: with the new launch/attach flow, we set the breakpoints in the dummy target before the actual target is created. If the target is created through launch commands, we're calling SetTarget with the currently selected target after we're done running the launch commands. By that time, we may have already missed events belong to the new target.

With this PR, we mimic what the default event handler does, which is listen to all target events. I think the problem with this approach is that now we're going to be broadcasting events for targets that are not the focus of the current debug session (i.e. the selected target). It should be easy to verify this by running a script that creates a second target and seeing if we now broadcast the events. If my theory is correct, then we would have to filter events not belonging to the selected target.

I had thought about also including a check on the target to see if it was the DAP.target, but again during launch or attach commands we may not know which target we need to focus on yet.

Interestingly, I was thinking about multi-target/multi-process support in DAP earlier this week. I had someone reach out to ask questions about this. I'll file an issue to track this, but we should keep that use case in mind as we design this.

We do support the startDebugging reverse request https://microsoft.github.io/debug-adapter-protocol/specification#Reverse_Requests_StartDebugging. I use this to attach to multiple processes when tests are running from some scripts that involve multiple processes. I use the script hook we have

I had thought about also including a check on the target to see if it was the DAP.target, but again during launch or attach commands we may not know which target we need to focus on yet.

Can we use the selected target until we've decided on the dap target? I guess that still leaves the possibility that the launch commands create multiple targets and that you temporarily see events belonging to the wrong target before another target was selected, but I'm not sure how to avoid that. Unless there's a way to queue up events? I'd need to look at the code in more detail to figure out how that works exactly (or ask @jimingham).

We do support the startDebugging reverse request https://microsoft.github.io/debug-adapter-protocol/specification#Reverse_Requests_StartDebugging. I use this to attach to multiple processes when tests are running from some scripts that involve multiple processes. I use the script hook we have

llvm-project/lldb/tools/lldb-dap/DAP.h

Line 135 in 680b3b7

struct StartDebuggingRequestHandler : public lldb::SBCommandPluginInterface {

to call this from a python script. With the lldb-dap server mode we can handle multiple DAP sessions from the same binary, or you can have lldb-dap launch one instance per debug session. The DAP doesn't really have a way to have multiple processes per debug session, but it does allow for multiple debug sessions in general. Compound launch configurations are one way to get this setup https://code.visualstudio.com/docs/debugtest/debugging-configuration#_compound-launch-configurations for example, you could have a server and a client in a compound launch configuration.

Cool, I knew we supported the request but I didn't know how that was hooked up. So it seems like this may be better supported than I thought. In that case we should definitely make sure we test that configuration and make sure it keeps working.

Without reading the discussion, I also started thinking about the multi-target/multi-debugger scenario.

In principle, I don't see a reason why we couldn't queue up breakpoint events (either inside the listener or inside some external queue) until we know which target we are dealing with, but it seems kinda wasteful to be subscribing to all events when we're only interested in a specific target.

Is it possible to delay setting the breakpoint until we know what the target is? Or will that be a problem because attach/launchCommands will already resume the process? Could we require that the target is created inside preRunCommands or initCommands?

Is it possible to delay setting the breakpoint until we know what the target is? Or will that be a problem because attach/launchCommands will already resume the process? Could we require that the target is created inside preRunCommands or initCommands?

Previously, we would handle the launch/attach request when it arrived, which meant we'd have a target configured by the time the setBreakpoint request was called. Also previously, we would leave the process in a stopped state until the configurationDone request was sent. #138219 adjusted that to delay handling the launch/attach until we get the configuration done. https://discourse.llvm.org/t/reliability-of-the-lldb-dap-tests/86125 has some background on that change.

During the startup flow we do need to reply to the setBreakpoint requests before VSCode will send the configurationDone. We could just mark the breakpoints as unverified and not 'really' set them until we finish launching. That could also work around this issue.

Can we use the selected target until we've decided on the dap target? I guess that still leaves the possibility that the launch commands create multiple targets and that you temporarily see events belonging to the wrong target before another target was selected, but I'm not sure how to avoid that. Unless there's a way to queue up events? I'd need to look at the code in more detail to figure out how that works exactly (or ask @jimingham).

Another possible way to handle this would be to iterate over the known breakpoints after the attachCommands/launchCommands have finished and check if we need to update the breakpoint state.

For reference, our launch flow we use is roughly:

{
    "type": "lldb-dap",
    "request": "launch",
    "name": "...",
    "initCommands": [
      "command source config/lldbinit" // loads python scripts with additional helpers
    ],
    "launchCommands": [
        "!launch --device <ios-simulator-id> bazel-bin/path/to/ios_app.ipa"
    ],
    "preLaunchTask": "bazel build //path/to:ios_app"
},

The python script is doing roughly:

• unzipping the ipa
• running simctl install / devicectl install
• debugger.CreateTarget('<install-path>', '', '<platform>', True)
• simctl launch --wait-for-debugger / devicectl device process launch --start-stopped
• debugger.GetSelectedTarget().AttachToProcessWithID(<pid>)

Okay, to summarize my previous comment, I think we could address this in the following ways:

1. Listen for the broadcast class instead of a specific broadcaster. The downside here is we may see events for targets/processes we're not directly interested in.
2. In the setBreakpoint (and friends) functions, delay actually setting the breakpoint until after configurationDone. The downside here is the added complexity to our startup flow.
3. After launchCommands/attachCommands, check the existing breakpoints and update the state. The downside here is ensuring we've correctly checked all stateful values after the launch is complete.

#140331 will also fix this underlying issue. I think its a better approach to fix this without having to listen for all events.